Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add PeerDAS DataColumn type #5913

Merged
merged 2 commits into from
Jul 9, 2024
Merged

Conversation

dapplion
Copy link
Collaborator

@dapplion dapplion commented Jun 11, 2024

Issue Addressed

First step to progressively merge das into unstable. Almost all changes use the DataColumnSidecar type so its a pre-requisite for anything else.

DataColumnSidecar includes some methods that require a newer version of ckzg-4844. Currently, these changes live in the das branch https://github.com/ethereum/c-kzg-4844/tree/das. I propose to merge the DataColumnSidecar with no-op crypto until c-kzg-4844 changes land on main, which should happen in < 2 months.

Since this PR does not introduce any actual functionality of PeerDAS it does not make a big difference to have the actual crypto backend. Once we merge enough pieces of the das branch to be able to run a PeerDAS node we can switch to c-kzg-4844/das.

Proposed Changes

Introduce the DataColumnSidecar type from

Upstream PRs

@dapplion dapplion added ready-for-review The code is ready for review das Data Availability Sampling labels Jun 11, 2024
@dapplion
Copy link
Collaborator Author

There has been very little interest in progressively merging PeerDAS in atomic PRs so I wave a white flag and join the group of yoloing das into unstable

@dapplion dapplion closed this Jun 27, 2024
@jimmygchen
Copy link
Member

I thought I already waved mine a while ago 😅
I think @pawanjay176 might have started looking into this, I'll bring it up again to make sure we keep the ball rolling.

Copy link
Member

@pawanjay176 pawanjay176 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies for dragging my feet on this. Would very much prefer the split up PRs.
This looks mostly good

consensus/types/src/beacon_block_body.rs Outdated Show resolved Hide resolved
@jimmygchen jimmygchen reopened this Jun 28, 2024
@dapplion
Copy link
Collaborator Author

What a comeback! 🔥 We should include Kev's lib in this PR to have mock crypto

Co-authored-by: Jacob Kaufmann <jacobkaufmann18@gmail.com>
Co-authored-by: Jimmy Chen <jchen.tc@gmail.com>
Copy link
Member

@pawanjay176 pawanjay176 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

type KzgCommitmentInclusionProofDepth = U17;
type KzgCommitmentsInclusionProofDepth = U4; // inclusion of the whole list of commitments
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just noting that the naming here is really confusing (w.r.t KzgCommitmentInclusionProofDepth) and has a potential for logic errors. Maybe we can address the naming in the spec

Copy link
Member

@realbigsean realbigsean left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@realbigsean
Copy link
Member

@mergify queue

Copy link

mergify bot commented Jul 9, 2024

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 15985b8

mergify bot added a commit that referenced this pull request Jul 9, 2024
@mergify mergify bot merged commit 15985b8 into sigp:unstable Jul 9, 2024
28 checks passed
@dapplion dapplion deleted the das-columns-type branch July 10, 2024 09:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
das Data Availability Sampling ready-for-review The code is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants